Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

POC protobuf communication over FFI #626

Merged
merged 1 commit into from
Feb 13, 2019
Merged

POC protobuf communication over FFI #626

merged 1 commit into from
Feb 13, 2019

Conversation

eoger
Copy link
Contributor

@eoger eoger commented Feb 6, 2019

See #612 for discussion.

@eoger eoger requested review from thomcc and a team February 6, 2019 20:57
Copy link
Contributor

@thomcc thomcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot on this! This looks pretty cool.

I'd like doc comments on the ffi_support changes. Also, could you do a version bump on it when you're done?

I also think we should look into Prost, since the set_blah boilerplate is a bit of step backwards IMO. It seems pretty actively maintained, actually.

Also, the Int/Long distinction is a big issue too.

components/support/ffi/src/lib.rs Outdated Show resolved Hide resolved
components/support/ffi/src/lib.rs Outdated Show resolved Hide resolved
components/support/ffi/src/macros.rs Outdated Show resolved Hide resolved
components/fxa-client/src/ffi.rs Outdated Show resolved Hide resolved
components/support/ffi/src/lib.rs Outdated Show resolved Hide resolved
components/support/ffi/src/lib.rs Outdated Show resolved Hide resolved
@eoger eoger force-pushed the protobufs branch 3 times, most recently from 35fa958 to 041f0b7 Compare February 8, 2019 21:34
@eoger
Copy link
Contributor Author

eoger commented Feb 8, 2019

Mind having a look again?
Notable changes:

  • Switched to the Prost crate.
  • Docs docs docs.
  • Added Swift support.

Copy link
Contributor

@thomcc thomcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for your work on this, and I'm sorry that I have so many review comments.

I'd like to see it again, though, one last time.

components/support/ffi/src/lib.rs Outdated Show resolved Hide resolved
components/support/ffi/src/lib.rs Outdated Show resolved Hide resolved
components/fxa-client/Cargo.toml Show resolved Hide resolved
components/fxa-client/src/ffi.rs Outdated Show resolved Hide resolved
components/fxa-client/src/ffi.rs Outdated Show resolved Hide resolved
components/fxa-client/ios/FxAClient/fxa.h Outdated Show resolved Hide resolved
components/fxa-client/src/ffi_types.proto Show resolved Hide resolved
components/fxa-client/src/ffi_types.proto Outdated Show resolved Hide resolved
components/support/ffi/src/lib.rs Show resolved Hide resolved
components/support/ffi/src/macros.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@vladikoff vladikoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments, seems to work locally, built with Android Studio locally

.travis.yml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
build.gradle Show resolved Hide resolved
components/fxa-client/android/build.gradle Show resolved Hide resolved
components/fxa-client/android/build.gradle Show resolved Hide resolved
components/fxa-client/build.rs Show resolved Hide resolved
components/fxa-client/ffi/src/lib.rs Show resolved Hide resolved
components/fxa-client/src/ffi.rs Show resolved Hide resolved
@eoger eoger force-pushed the protobufs branch 2 times, most recently from 2a669b3 to e8a82eb Compare February 12, 2019 22:15
Copy link
Contributor

@thomcc thomcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good, if you can do the convert_protobuf_to_byte_buffer inside the macro i'd prefer that, but if not it's fine.

components/support/ffi/src/macros.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants